fix(tasks): accept optional groups in the regex screen and bound RegexTask matching - #848

Two coupled issues in the ReDoS defence. They ship together because fixing the first makes the second worse.

1. The shape screen rejected safe, common patterns

scanPattern (packages/tasks/src/util/regexSafety.ts) treated ? — and a bare { — after ) as "the group is quantified", so any group whose body contains +/* and is then made optional tripped nestedQuantifiers. (X)? bounds the group to at most one repetition, so there is nothing for the engine to backtrack over and the match stays linear. A pure false positive, and it gated three user-facing tasks: FileGrepTask, FileSedTask, RegexTask.

Confirmed rejected-but-safe before this change:

^-?\d+(\.\d+)?$    ^-?\d+(?:\.\d+)?$    (\w+)?    ^(#.*)?$
(\s+)?end          (https?://\S+)?      (ERROR|WARN)\s+(\w+)?

The correct predicate is "does the quantifier permit two or more repetitions", added as quantifierAllowsRepeat beside isUnboundedRepetitionAt (which is left alone — it answers the body question, "can this match variable lengths", where X{10} is fixed-length and fine).

The tempting cheaper variant — reuse isUnboundedRepetitionAt for the group test, i.e. require a real {n,}/{n,m} instead of a bare { — was rejected: its regex requires a comma, so it would start accepting (a+){10}, measured at 46,318 ms against a 41-character input, and would flip the existing must-reject case (a+){2}. it.each(["(a+){2}", "(a+){2,3}", "(a+){10}", "(a+){2,}"]) pins that.

Sticky exec throughout, never test(pattern.slice(...)) — slicing per character is the quadratic shape the module opens by explaining it exists to avoid.

Newly accepted (all verified against the real module)

^-?\d+(\.\d+)?$, ^-?\d+(?:\.\d+)?$, (\w+)?, ^(#.*)?$, (\s+)?end, (https?://\S+)?, (ERROR|WARN)\s+(\w+)?, (a+)?, (?<year>\d+)?, (a+){1}, (a+){0,1}, (a+){foo} — plus the already-passing (\d)?, ^(\d{4})-(\d{2})-(\d{2})$, (foo|far)+, ([a-z]|[A-Z])*, foo.*bar, (a\+)+, ([*+])+, (?:\r?\n)+, ^(?:https?://)?(www\.)?example\.com.

Still rejected (all verified)

(a+)+, (a*)+, (a+)*, (a+){2}, ((b|c+))+, (a+)+$, (a*)*$, (x+x+)+y, ([a-z]+)+$, ((a)*)*$, (\d+)+$, (a|a)*$, (?:a|a)*$, (a|a|a)+$, ^(a|ab)+$, (a*|b)+$, (a{2,})*$, ([a-z]|[a-z])*, \[(a+)+], ([a-z]*|[A-Z])+, (a+){2,3}, (a+){10}, (a+){2,}, ((a+)?)+.

((a+)?)+ is the interesting one: the inner (a+)? no longer trips the rule, but "a quantifying group counts as a quantifier for whatever encloses it" still marks the outer group, so it stays rejected. Pinned by its own test.

One intentional expectation flip: "(a+)?" was removed from it("rejects nested quantifiers").

2. RegexTask matched unbounded

RegexTask called assertSafeRegexPattern and then new RegExp + exec/matchAll on the calling thread. The screen's own JSDoc says a false is not a safety guarantee and that the enforced containment is the match budget in createBoundedRegexMatcher — which RegexTask never used. ^(a?b?)*$ passes the screen (no group quantifies, no alternation overlaps) and measured 31,714 ms against "ab".repeat(28) + "!". executePreview ran the same unbounded code. Relaxing the screen widens what reaches this path, hence one PR.

A RegexTask.server subclass is not viable: TaskRegistry.registerTask throws on a second class for the same type, and RegexTask is registered from common.ts. So this uses the injection seam the package already has for safeFetch (SafeFetch.ts + SafeFetch.server.ts tail + a side-effect import in node.ts):

• util/BoundedRegexRunner.ts (new, cross-platform) — RegexRunner / RegexRunnerFactory, plus registerRegexRunnerFactory / getRegexRunnerFactory / resetRegexRunnerFactory, the same three-function shape as SafeFetch.ts. defaultRegexRunnerFactory does the plain unbounded exec: the browser default, where a hostile pattern blocks the tab rather than a host process — the trade-off already documented on FileGrepTask.createLineMatcher.
• util/BoundedRegex.server.ts — createBoundedRegexExecutor(timeoutMs), using one module-level createContext + Script with re assigned per call. Measured: shared context 0.16 ms/call vs 0.74 ms for a fresh one, and RegexTask evaluates a single value per run, so the per-call-site context createBoundedRegexMatcher builds is the wrong shape here. Results are copied out with Array.prototype.slice.call(r) so an unmatched optional group stays undefined in place rather than being dropped; the zero-length-match lastIndex++ guard is reused from createBoundedRegexExtractor. Timeout throws TaskInvalidInputError with the existing message shape — deliberately not TaskTimeoutError, which extends TaskAbortedError and would report the run aborted rather than failed by bad input. The file ends with the registerRegexRunnerFactory(...) call at SECURITY_LIMITS.regexMatchBatchTimeoutMs.
• node.ts / electron.ts — side-effect import next to SafeFetch.server. common.ts re-exports the runner module.
• RegexTask.ts — executeRegex now uses compileSafeRegex (so a bad flag raises TaskInvalidInputError instead of a raw SyntaxError) and takes its matcher from getRegexRunnerFactory(). This also collapses the double new RegExp the global path was doing. Both execute and executePreview go through it.

Tests

^((a?)(b?))*$ is the budget test pattern precisely because it is invisible to the screen both before and after this change — it proves the budget, not the screen. Unbounded it measures 5,829 ms against "ab".repeat(28) + "!"; the budget is 1,000 ms. It now terminates at ~1,007 ms. Bun honours a vm timeout coarsely, so the assertion leaves headroom at 5,000 ms.

The nine existing RegexTask assertions are the regression suite for the runner swap and pass unchanged.

 ✓ RegexSafety.test.ts > assertSafeRegexPattern > rejects nested quantifiers
 ✓ RegexSafety.test.ts > hasUnsafeRegexShape > accepts the optional group ^-?\d+(\.\d+)?$
 ✓ RegexSafety.test.ts > hasUnsafeRegexShape > accepts the optional group (a+)?
 ✓ RegexSafety.test.ts > hasUnsafeRegexShape > still rejects the counted repetition (a+){10}
 ✓ RegexSafety.test.ts > hasUnsafeRegexShape > still rejects an optional quantifying group under an outer quantifier
 ✓ RegexSafety.test.ts > hasUnsafeRegexShape > stays linear on a newly accepted optional group 4ms
 ✓ RegexTask.test.ts   > RegexTask > fails a catastrophically backtracking pattern instead of blocking 1009ms
 ✓ RegexTask.test.ts   > RegexTask > accepts a decimal pattern the shape screen used to reject 1ms

 Test Files  2 passed (2)
      Tests  61 passed (61)

Full section, against the built dist (bun run build:packages then bun scripts/test.ts task vitest):

Running all tests in sections [task] — 74 file(s)

 Test Files  74 passed (74)
      Tests  1246 passed | 24 skipped (1270)
   Duration  217.45s

bunx tsc -p packages/tasks/tsconfig.json --noEmit is clean, and bun run format reports no changes.

🤖 Generated with Claude Code

https://claude.ai/code/session_01LowBJQsCghLDiHwPN6FgUT

Generated by Claude Code